Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-6508] Update the version of cattrs from 0.9 to 1.0 #7100

Merged
merged 1 commit into from
Jan 8, 2020

Conversation

sarutak
Copy link
Member

@sarutak sarutak commented Jan 8, 2020

cattrs 0.9 with Python 3.8 causes following error.

$ airflow
Traceback (most recent call last):
  File "/Users/sarutak/airflow-env/bin/airflow", line 27, in <module>
    from airflow.bin.cli import CLIFactory
  File "/Users/sarutak/airflow-env/lib/python3.8/site-packages/airflow/__init__.py", line 40, in <module>
    from airflow.models.dag import DAG
  File "/Users/sarutak/airflow-env/lib/python3.8/site-packages/airflow/models/__init__.py", line 21, in <module>
    from airflow.models.baseoperator import BaseOperator, BaseOperatorLink  # noqa: F401
  File "/Users/sarutak/airflow-env/lib/python3.8/site-packages/airflow/models/baseoperator.py", line 41, in <module>
    from airflow.lineage import apply_lineage, prepare_lineage
  File "/Users/sarutak/airflow-env/lib/python3.8/site-packages/airflow/lineage/__init__.py", line 28, in <module>
    from cattr import structure, unstructure
  File "/Users/sarutak/airflow-env/lib/python3.8/site-packages/cattr/__init__.py", line 2, in <module>
    from .converters import Converter, UnstructureStrategy
  File "/Users/sarutak/airflow-env/lib/python3.8/site-packages/cattr/converters.py", line 3, in <module>
    from ._compat import (
  File "/Users/sarutak/airflow-env/lib/python3.8/site-packages/cattr/_compat.py", line 86, in <module>
    from typing import _Union
ImportError: cannot import name '_Union' from 'typing' (/opt/python/3.8.0/lib/python3.8/typing.py) 

This issue is resolved in cattrs 1.0.
https://github.com/Tinche/cattrs/pull/73

So let's update it.


Issue link: AIRFLOW-6508

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@codecov-io
Copy link

Codecov Report

Merging #7100 into master will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7100      +/-   ##
==========================================
- Coverage   85.15%   84.87%   -0.29%     
==========================================
  Files         680      680              
  Lines       38824    38824              
==========================================
- Hits        33061    32952     -109     
- Misses       5763     5872     +109
Impacted Files Coverage Δ
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 78.75% <0%> (-20%) ⬇️
airflow/contrib/operators/ssh_operator.py 84.61% <0%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe20ef6...ca05904. Read the comment docs.

@potiuk potiuk merged commit 57745b8 into apache:master Jan 8, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Jan 8, 2020

Awesome work, congrats on your first merged pull request!

galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
@danieltahara
Copy link

Can you issue a patch version bump? This was merged Jan 8 but I still can't use it :(

@danieltahara
Copy link

How is this still not in 1.10rcs? @potiuk @feluelle

@potiuk
Copy link
Member

potiuk commented Apr 9, 2020

@kaxil -> could we merge this in rc5 as well? This is also a requirement-only change as per rc5 sqlalchemy ? I think it is pretty safe.

@potiuk
Copy link
Member

potiuk commented Apr 9, 2020

On the other hand... If this is only for python 3.8 compatibility, then maybe better to do it in 1.10.11.-> then we could add support to python 3.8 in master and test it in 1.10.11 as well. This is a major version change for cattrs, so there is a risk some obscure problems might occur.

@danieltahara -> we have not yet officially got Python 3.8 compatibility yet not even in master: https://github.com/apache/airflow/blob/master/README.md#master-version-200dev , and there is high chance other things will break with python 3.8 so I think It's better to wait for it until we officially add (and run all tests) for python 3.8. In the meantime 3.6 and 3.7 are also very good choices.

@kaxil
Copy link
Member

kaxil commented Apr 9, 2020

Yes, we don't support Python 3.8 at the moment. Last I check cattrs was still buggy for 1.0.0 & Py3.8

So it will definitely have to wait for next release.

@danieltahara
Copy link

Hi all,

Thanks for the responses. I think I'm finding it confusing that a commit would get merged into master and not used. If it doesn't break 3.6 and 3.7 tests (which it doesn't, since it got merged), and 3.8 is build your own adventure anyway (b/c it's not officially supported), then I don't see the harm in including it in the release.

I understand you have a very specific release branch flow, but the idea a commit can sit for 4 months on master seems like overhead for y'all (cherrypicking, mental overhead of remembering NOT to include the commit) and confusion for the public.

That said, I completely respect that outcome and really appreciate the work you do maintaining the project. Thanks!

@potiuk
Copy link
Member

potiuk commented Apr 9, 2020

Jut to explain - The flow is only because we are now in a process of working on backwards incompatible 2.0 release but in the meantime we release bugfixes and even some features in 1.10 line. It is an overhead indeed but we have to be mindful to our users who want to still have bugs solved and some minor features added and 2.0 version is going to be a bit more difficult to upgrade for them (and it can take months or years because of corporate limitations).

And we do not have to remember of not cherry-picking BTW - we only cherry-pick what we think is worth it. Most of the changes from master branch are not cherry-picked to 1.10.

Surprisingly - it works quite well so far.

@kaxil
Copy link
Member

kaxil commented Apr 9, 2020

To add to what Jarek said, we support Python 2 on Airflow 1.10.* branches, while we don't in Master so, makes cherry-picking process a bit harder for now but provides a good balance between development with new versions will backporting imp features.

@kaxil kaxil added this to the Airflow 1.10.11 milestone Jun 20, 2020
kaxil pushed a commit that referenced this pull request Jun 22, 2020
potiuk pushed a commit that referenced this pull request Jun 29, 2020
kaxil pushed a commit that referenced this pull request Jul 1, 2020
cmlad pushed a commit to cmlad/airflow that referenced this pull request Oct 27, 2020
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants